Skip to content

Fix Box allocator drop elaboration #143672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix Box allocator drop elaboration #143672

wants to merge 3 commits into from
+550 −26

Conversation

beepster4096
Copy link
Contributor

@beepster4096 beepster4096 commented Jul 9, 2025 •
edited by rustbot
Loading

New version of #131146.

Clearing Box's drop flag after running its destructor can cause it to skip dropping its allocator, so just don't. Its cleared by the drop ladder code afterwards already.

Unlike the last PR this also handles other types with destructors properly, in the event that we can have open drops on them in the future (by partial initialization or DerefMove or something).

Finally, I also added tests for the interaction with async drop here but I discovered #143658, so one of the tests has a knownbug annotation. Not sure if it should be in this PR at all though.

Fixes #131082

r? wesleywiser - prev. reviewer

@rustbot
Copy link
Collaborator

rustbot commented Jul 9, 2025

wesleywiser is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 9, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@beepster4096
Copy link
Contributor Author

r? compiler since wesleywiser is off rotation

@rustbot rustbot assigned WaffleLapkin and unassigned wesleywiser Jul 9, 2025
@WaffleLapkin
Copy link
Member

r? compiler

@rustbot rustbot assigned fee1-dead and unassigned WaffleLapkin Jul 15, 2025
@fee1-dead
Copy link
Member

r? compiler

@rustbot rustbot assigned oli-obk and unassigned fee1-dead Jul 16, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Jul 16, 2025

lmao I should have just taken this PR when I saw it right when it was opened

oli-obk
oli-obk reviewed Jul 16, 2025
View reviewed changes
compiler/rustc_mir_transform/src/elaborate_drop.rs Outdated
// types with destructors still need an empty drop ladder to clear it

// currently no rust types can trigger this path in a context where drop flags exist
// however, a future box-like "DerefMove" trait would allow it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so... should we span_bug! here until such a time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I should clarify the comment. This codepath is taken with some types today, but only with DropShimElaborator and not ElaborateDropsCtxt. It would be nice to panic here with ElaborateDropsCtxt only but I'm not sure how to accomplish that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just adjusting the comment is sufficient for me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered what I was going to do to make the span_bug work

@oli-obk
Copy link
Contributor

oli-obk commented Jul 21, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 21, 2025

📌 Commit 4f5f34c has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2025
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jul 21, 2025
@jieyouxu
Rollup merge of rust-lang#143672 - beepster4096:box_drop_flags_again,…
33800e5 
… r=oli-obk

Fix Box allocator drop elaboration

New version of rust-lang#131146.

Clearing Box's drop flag after running its destructor can cause it to skip dropping its allocator, so just don't. Its cleared by the drop ladder code afterwards already.

Unlike the last PR this also handles other types with destructors properly, in the event that we can have open drops on them in the future (by partial initialization or DerefMove or something).

Finally, I also added tests for the interaction with async drop here but I discovered rust-lang#143658, so one of the tests has a `knownbug` annotation. Not sure if it should be in this PR at all though.

Fixes rust-lang#131082

r? wesleywiser - prev. reviewer
@jieyouxu jieyouxu mentioned this pull request Jul 21, 2025
Closed
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jul 21, 2025
@jieyouxu
Rollup merge of rust-lang#143672 - beepster4096:box_drop_flags_again,…
20c4d33 
… r=oli-obk

Fix Box allocator drop elaboration

New version of rust-lang#131146.

Clearing Box's drop flag after running its destructor can cause it to skip dropping its allocator, so just don't. Its cleared by the drop ladder code afterwards already.

Unlike the last PR this also handles other types with destructors properly, in the event that we can have open drops on them in the future (by partial initialization or DerefMove or something).

Finally, I also added tests for the interaction with async drop here but I discovered rust-lang#143658, so one of the tests has a `knownbug` annotation. Not sure if it should be in this PR at all though.

Fixes rust-lang#131082

r? wesleywiser - prev. reviewer
@jieyouxu jieyouxu mentioned this pull request Jul 21, 2025
Closed
bors added a commit that referenced this pull request Jul 21, 2025
@bors
Auto merge of #144262 - jieyouxu:rollup-wxgtyr0, r=jieyouxu
006f365 
Rollup of 15 pull requests

Successful merges:

 - #142097 (gpu offload host code generation)
 - #143430 (Lower extra lifetimes before normal generic params.)
 - #143672 (Fix Box allocator drop elaboration)
 - #143768 (Constify Try, From, TryFrom and relevant traits)
 - #143816 (Implement `check` for compiletest and RA using tool macro)
 - #143985 (rustc_public: de-StableMIR-ize)
 - #144027 (clippy: make tests work in stage 1)
 - #144080 (Mitigate `#[align]` name resolution ambiguity regression with a rename)
 - #144176 (Add approval blocking labels for new bors)
 - #144187 (fix handling of base address for TypeId allocations)
 - #144212 (Remove the ptr_unique lang item)
 - #144243 (Subtree update of `rust-analyzer`)
 - #144246 (Don't use another main test file as auxiliary)
 - #144251 (rustc-dev-guide subtree update)
 - #144254 (opt-dist: make `artifact-dir` an absolute path for `opt-dist local`)

r? `@ghost`
`@rustbot` modify labels: rollup
@jieyouxu
Copy link
Member

jieyouxu commented Jul 21, 2025 •
edited
Loading

I believe this is missing a test change on test-various or is somehow different: #144262 (comment)

---- [mir-opt] tests/mir-opt/box_conditional_drop_allocator.rs stdout ----
74   
75       bb6: {
76           StorageDead(_4);
- -         drop(_1) -> [return: bb7, unwind continue];
+ -         drop(_1) -> [return: bb7, unwind: bb13];
78 +         goto -> bb23;
79       }
80

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 21, 2025
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Box sometimes forgets to drop its allocator when the Box is conditionally initialized.
8 participants
@beepster4096 @rustbot @WaffleLapkin @fee1-dead @oli-obk @bors @jieyouxu @wesleywiser